-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Add Image Prefetching for Click to expand Images #61107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Add Image Prefetching for Click to expand Images #61107
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
68fcd2d
to
5c10b84
Compare
@Takshil-Kunadia Thanks for creating this PR! I've left a comment on the issue to loop a few other folks into the discussion who may have thoughts on how to best proceed. In short, we previously had prefetching on hover but removed it. Since then, there's been more discussion around this, so perhaps may be good to add this back. |
5c10b84
to
e6133b0
Compare
e6133b0
to
7593d56
Compare
Thanks @artemiomorales ! for the context and for looping the stakeholders in the discussion! I've updated the code to reflect the direction discussed in the issue. Happy to adjust further based on any additional feedback 😄 |
27aa823
to
6dd8ed5
Compare
I'm just realizing a problem here with the overall image lightbox: what if the original |
3e26db9
to
3ab6bc0
Compare
Thanks! @westonruter for looking into this 🙇🏻♂️ and yes good catch! it makes sense to have responsive images for lightbox images to avoid unnecessarily large downloads, especially on smaller viewports. One thing I’d like to highlight though is that That said, I’ve implemented the suggested preload approach in d7f2e41 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, just a couple of comments 🙂
…ad delay constant
const srcset = imageMetadata.lightboxSrcset; | ||
if ( srcset ) { | ||
imageLink.setAttribute( 'imagesrcset', srcset ); | ||
imageLink.setAttribute( 'imagesizes', '100vw' ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't enlargedSrcset
and enlargedSizes
be used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the getter functions, but for that we need to temporarily set the currentImageId
to use them and then reset the Id when we have preloaded the image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for that we need to temporarily set the currentImageId to use them and then reset the Id when we have preloaded the image
I don't think it would be a good approach, as there can be more than one image preloading at the same time.
Why don't you just use imageMetadata.lightboxSizes || '100vw'
instead of just '100vw'
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was how it was implemented previously. I changed it in cab1562 to use the getter fns.
Reverted this commit ↩️ along with the dynamic lightboxSizes
change
/** | ||
* Returns the appropriate src URL for an image | ||
* | ||
* @param {Object} imageMetadata Image metadata object | ||
* @return {string} The source URL | ||
*/ | ||
function getImageSrc( imageMetadata ) { | ||
return ( | ||
imageMetadata.uploadedSrc || | ||
'data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs=' | ||
); | ||
} | ||
|
||
/** | ||
* Returns the appropriate srcset for an image | ||
* | ||
* @param {Object} imageMetadata Image metadata object | ||
* @return {string} The srcset value | ||
*/ | ||
function getImageSrcset( imageMetadata ) { | ||
return imageMetadata.lightboxSrcset || ''; | ||
} | ||
|
||
/** | ||
* Returns the appropriate sizes attribute for an image | ||
* | ||
* @param {Object} imageMetadata Image metadata object | ||
* @return {string} The sizes value, defaulting to 100vw | ||
*/ | ||
function getImageSizes( imageMetadata ) { | ||
return imageMetadata.lightboxSizes || '100vw'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor improvement, to add typing for the passed prop:
/** | |
* Returns the appropriate src URL for an image | |
* | |
* @param {Object} imageMetadata Image metadata object | |
* @return {string} The source URL | |
*/ | |
function getImageSrc( imageMetadata ) { | |
return ( | |
imageMetadata.uploadedSrc || | |
'data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs=' | |
); | |
} | |
/** | |
* Returns the appropriate srcset for an image | |
* | |
* @param {Object} imageMetadata Image metadata object | |
* @return {string} The srcset value | |
*/ | |
function getImageSrcset( imageMetadata ) { | |
return imageMetadata.lightboxSrcset || ''; | |
} | |
/** | |
* Returns the appropriate sizes attribute for an image | |
* | |
* @param {Object} imageMetadata Image metadata object | |
* @return {string} The sizes value, defaulting to 100vw | |
*/ | |
function getImageSizes( imageMetadata ) { | |
return imageMetadata.lightboxSizes || '100vw'; | |
} | |
/** | |
* Returns the appropriate src URL for an image. | |
* | |
* @param {string} uploadedSrc - Full size image src. | |
* @return {string} The source URL. | |
*/ | |
function getImageSrc( { uploadedSrc } ) { | |
return ( | |
uploadedSrc || | |
'data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs=' | |
); | |
} | |
/** | |
* Returns the appropriate srcset for an image. | |
* | |
* @param {string} lightboxSrcset - Image srcset. | |
* @return {string} The srcset value. | |
*/ | |
function getImageSrcset( { lightboxSrcset } ) { | |
return lightboxSrcset || ''; | |
} | |
/** | |
* Returns the appropriate sizes attribute for an image. | |
* | |
* @param {string} lightboxSizes - Image responsive sizes attribute. | |
* @return {string} The sizes value, defaulting to 100vw. | |
*/ | |
function getImageSizes( { lightboxSizes } ) { | |
return lightboxSizes || '100vw'; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the changes ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the minor suggestion in #61107 (comment) this looks good to me. It is successfully preloading the image prior to opening it in the lightbox.
It can be hard to see, but in the before case the image is a bit blurry while the full size image is loaded:
Before | After |
---|---|
without-preload.mov |
with-preload.mov |
PR for #60883
What?
Prefetch full-resolution image to speed up display of Click to expand images.
see #60883 for more details
Why?
Hovering over a click-to-expand image block does not proactively attempt to prefetch the full-scale image in Lighbox. This can result in a prolonged period in which the low-resolution scaled-up image is displayed in the lightbox.
How?
pointerup
&pointerdown
.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast